Skip to content

fix: guard multiBufferBuilder UnsafeAppend against short copy#896

Open
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:fix-multibufferbuilder-unsafeappend
Open

fix: guard multiBufferBuilder UnsafeAppend against short copy#896
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:fix-multibufferbuilder-unsafeappend

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Harden multiBufferBuilder.UnsafeAppend to fail fast when a value copy is truncated.
  • After copy, validate that bytes copied equals the requested value length.
  • Keep the existing debug assertion path and add a runtime panic for non-assert builds.
  • Add regression coverage via TestMultiBufferBuilderUnsafeAppendPanicsOnTruncatedCopy in arrow/array/bufferbuilder_test.go.

Why

This prevents silent data corruption in BinaryViewBuilder and StringViewBuilder out-of-line storage where metadata can point past the bytes actually written.

Testing

  • Added regression test for the truncation panic path.
  • Full suite not run for this defensive-path-only change.

@fallintoplace fallintoplace requested a review from zeroshade as a code owner July 4, 2026 00:33

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two issues:

1. The new test doesn't trigger the guard — it fails. multiBufferBuilder.Reserve sizes a new block to max(nbytes, blockSize) and memory.Buffer.Reserve rounds capacity up to a multiple of 64, so Reserve(4) with blockSize: 8 gives a 64-byte block. Appending 9 bytes fits, so there's no short copy and the panic never fires:

--- FAIL: TestMultiBufferBuilderUnsafeAppendPanicsOnTruncatedCopy
    should panic ... Panic value: <nil>

2. Redundant guard. debug.Assert(n == len(val), ...) followed by an unconditional if n != len(val) { panic(...) } is doubled up — the debug.Assert is dead once the unconditional panic exists. And since Reserve always rounds the block up and the contract is that callers Reserve before UnsafeAppend, a short copy is effectively unreachable via the normal path — worth deciding whether an always-on branch belongs on this hot Unsafe path (vs. a debug.Assert only).

If you keep the guard, the test needs a genuinely undersized block (append a value larger than the block's rounded capacity). Verified against the PR head with go test.

@zeroshade

Copy link
Copy Markdown
Member

@fallintoplace gentle nudge — still blocked on the failing test: because multiBufferBuilder.Reserve rounds the block up to a multiple of 64, the 9-byte append into an 8-byte-requested block never short-copies, so assert.Panics fails (Panic value: <nil>). Details in my review above. Ping me and I'll re-review once it's updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants